Mesh::face_arities becomes Option<Vec<u32>> - #80
Conversation
LoadOptions gains progress_callback: Option<LoadProgressCallback> (Arc<dyn Fn(&LoadProgress) -> ControlFlow<()> + Send + Sync>, an options field rather than a second _with_progress function), invoked every 1000 lines during load_obj_buf's parse loop. Returning ControlFlow::Break stops the load and returns the new LoadError::Cancelled. No behavior change when progress_callback is None (the default): verified by a test comparing output with and without a no-op callback. load_obj_buf_async is untouched.
None now means "all faces are triangles" instead of an empty Vec, saving
the allocation for triangle-only meshes -- the common case. New
Mesh::{face_count, face_arity, is_triangulated, face_indices} helpers
replace manual index-arithmetic over face_arities at call sites (see
print_mesh.rs and the module doc example). Also tidies two loops
(parse_face, TmpMaterials::extend) into iterator chains while in the area.
BREAKING CHANGE: any caller reading Mesh::face_arities directly (as a Vec)
needs to match on the Option, or switch to the new helper methods.
|
I think bumping the semver to indicate the breaking API change sounds good, I'd put these both out in the same release |
Twinklebear
left a comment
Twinklebear
left a comment
There was a problem hiding this comment.
I think adding the new convenience methods on the Mesh is useful but we don't need to change the face_arities to an Option, since an empty Vec already doesn't allocate: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.new so it's just an API breaking change w/o much benefit.
|
It may not allocate but it's just a cleaner API IMHO. |
2 participants
Summary
Stacks on #79 (progress-callback) — this diff includes both commits until #79 merges; the second commit is the one this PR is actually about.
Mesh::face_aritieschanges fromVec<u32>toOption<Vec<u32>>.Nonenow means "all faces are triangles" instead of an emptyVec, which skips the allocation for triangle-only meshes — the common case, especially withtriangulate: true.Mesh::{face_count, face_arity, is_triangulated, face_indices}helper methods replace manual index-arithmetic overface_aritiesat call sites (see the updatedexamples/print_mesh.rsand the crate's module-doc example) — the offset math for finding a given face's indices was previously left to every caller to get right.parse_face,TmpMaterials::extend) into iterator chains while in the area — no behavior change there.BREAKING CHANGE: any caller reading
Mesh::face_aritiesdirectly as aVecneeds to match on theOption, or switch to the new helper methods. Happy to gate this behind a semver-major bump on your end, or split differently if you'd rather land it separately from #79 for that reason.Test plan
cargo test --all-features: 21 lib tests + 5 doctests pass, including updatednon_triangulated_quadcoverage for theNone/Somecasescargo build --example print_mesh --all-featurescargo fmt --all -- --check: cleancargo clippy --all-targets --all-features -- -D warnings: clean (exit 0)